Use %w instead of %v to format errors #99389

tanjing2020 · 2021-02-24T06:17:09Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Ref #94437

Special notes for your reviewer:

@alculquicondor

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-02-24T06:17:16Z

@tanjing2020: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-02-24T06:17:17Z

Hi @tanjing2020. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tanjing2020 · 2021-02-24T06:19:05Z

/release-note-none

k8s-ci-robot · 2021-02-24T09:16:11Z

@tanjing2020: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lala123912 · 2021-02-24T09:19:00Z

/ok-to-test

chendave

I guess the commit subject need to be updated, iiuc, this is not wrapping anything, and most of case "%w" is identical to "%v" but seems the "%w" is better under some situation.

tanjing2020 · 2021-02-24T12:41:32Z

/recc @chendave

gavinfish

%w is never better than %v, and the difference is that whether we want to expose the underlying errors to callers. So please check one by one instead of search and replace all.

alculquicondor · 2021-02-24T14:37:05Z

%w is never better than %v

Why so? There are certain scenarios where we need to compare underlying errors. Just migrating everything to wrapped errors keeps consistency, so that we are ready to check errors when needed.

alculquicondor

To keep this PR focused, limit the changes to pkg/scheduler/framework, as those are the errors we are more likely to care about being wrapped.

pkg/scheduler/framework/plugins/noderesources/fit.go

gavinfish · 2021-02-24T15:37:34Z

%w is never better than %v

Why so? There are certain scenarios where we need to compare underlying errors. Just migrating everything to wrapped errors keeps consistency, so that we are ready to check errors when needed.

aha, what I want to say is %w is not always better than %v indeed. Yes, we should expose some underlying errors case by case.

alculquicondor · 2021-02-24T16:23:48Z

I don't think picking is worth the effort, given that the downsides of migrating everything are negligible.

alculquicondor · 2021-02-25T14:22:10Z

pkg/scheduler/framework/plugins/serviceaffinity/service_affinity.go

@@ -108,14 +108,14 @@ func (pl *ServiceAffinity) createPreFilterState(pod *v1.Pod) (*preFilterState, e
 	// Store services which match the pod.
 	matchingPodServices, err := helper.GetPodServices(pl.serviceLister, pod)
 	if err != nil {
-		return nil, fmt.Errorf("listing pod services: %v", err.Error())
+		return nil, fmt.Errorf("listing pod services: %w", err.Error())


oh, don't call err.Error(), because that's a string

I was too careless to notice the problem. Thank you very much. It has been revised.

alculquicondor · 2021-02-26T14:30:09Z

you have to fix the tests

gavinfish · 2021-02-27T09:59:54Z

pkg/scheduler/framework/plugins/interpodaffinity/filtering_test.go

 	"reflect"
 	"strings"
 	"testing"

-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"


nit: this change is unnecessary.

alculquicondor · 2021-03-01T15:11:43Z

pkg/scheduler/framework/plugins/serviceaffinity/service_affinity_test.go

@@ -618,7 +619,7 @@ func TestPreFilterDisabled(t *testing.T) {
 	}
 	cycleState := framework.NewCycleState()
 	gotStatus := p.Filter(context.Background(), cycleState, pod, nodeInfo)
-	wantStatus := framework.AsStatus(fmt.Errorf(`error reading "PreFilterServiceAffinity" from cycleState: not found`))
+	wantStatus := framework.AsStatus(fmt.Errorf(`error reading "PreFilterServiceAffinity" from cycleState: %w`, errors.New("not found")))


Instead, export the error in framework/cycle_state.go? That is the benefit of wrapping, that we can compare the exact error. Now, we shouldn't be using reflect, but that's a separate problem.

tanjing2020 · 2021-03-05T03:05:18Z

/retest

tanjing2020 · 2021-03-05T06:06:17Z

/retest

tanjing2020 · 2021-03-05T08:03:37Z

/retest

alculquicondor · 2021-03-05T14:38:43Z

pkg/scheduler/framework/plugins/noderesources/fit_test.go

@@ -437,7 +438,7 @@ func TestPreFilterDisabled(t *testing.T) {
 	}
 	cycleState := framework.NewCycleState()
 	gotStatus := p.(framework.FilterPlugin).Filter(context.Background(), cycleState, pod, nodeInfo)
-	wantStatus := framework.NewStatus(framework.Error, `error reading "PreFilterNodeResourcesFit" from cycleState: not found`)
+	wantStatus := framework.AsStatus(fmt.Errorf(`error reading "PreFilterNodeResourcesFit" from cycleState: %w`, errors.New(framework.NotFound)))


can you make framework.NotFound an error instead of a string?

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2021-03-08T19:03:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, tanjing2020

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alculquicondor · 2021-03-08T19:38:40Z

/priority backlog

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 24, 2021

k8s-ci-robot requested review from chendave and wgliang February 24, 2021 06:18

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 24, 2021

chendave reviewed Feb 24, 2021

View reviewed changes

tanjing2020 force-pushed the change-log branch from 9fcd2fc to 902ee8f Compare February 24, 2021 10:08

tanjing2020 changed the title ~~Wrap errors in scheduler~~ Use %w instead of %v to format errors Feb 24, 2021

gavinfish reviewed Feb 24, 2021

View reviewed changes

alculquicondor reviewed Feb 24, 2021

View reviewed changes

pkg/scheduler/framework/plugins/noderesources/fit.go Show resolved Hide resolved

tanjing2020 force-pushed the change-log branch from 902ee8f to 6ca9bc4 Compare February 25, 2021 01:35

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 25, 2021

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 25, 2021

alculquicondor reviewed Feb 25, 2021

View reviewed changes

tanjing2020 force-pushed the change-log branch from 6ca9bc4 to f30518c Compare February 26, 2021 02:19

tanjing2020 force-pushed the change-log branch from f30518c to a9f7568 Compare February 27, 2021 03:03

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 27, 2021

gavinfish reviewed Feb 27, 2021

View reviewed changes

tanjing2020 force-pushed the change-log branch from a9f7568 to a01c8ae Compare March 1, 2021 03:24

alculquicondor reviewed Mar 1, 2021

View reviewed changes

tanjing2020 mentioned this pull request Mar 3, 2021

REQUEST: New membership for tanjing2020 kubernetes/org#2536

Closed

6 tasks

tanjing2020 force-pushed the change-log branch from b3a6fe8 to 8da11e4 Compare March 5, 2021 01:25

tanjing2020 force-pushed the change-log branch from 8da11e4 to 68ddddb Compare March 5, 2021 03:58

alculquicondor reviewed Mar 5, 2021

View reviewed changes

Use %w instead of %v to format errors

61cd099

tanjing2020 force-pushed the change-log branch from 68ddddb to 61cd099 Compare March 8, 2021 02:28

alculquicondor reviewed Mar 8, 2021

View reviewed changes

k8s-ci-robot assigned alculquicondor Mar 8, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2021

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 8, 2021

k8s-ci-robot merged commit 2b3f97b into kubernetes:master Mar 8, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 8, 2021

SataQiu mentioned this pull request Jul 25, 2022

kubeadm: prefer to use pkg/errors package and cleanup fmt.Errorf #111394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use %w instead of %v to format errors #99389

Use %w instead of %v to format errors #99389

tanjing2020 commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

tanjing2020 commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

lala123912 commented Feb 24, 2021

chendave left a comment

tanjing2020 commented Feb 24, 2021

gavinfish left a comment

alculquicondor commented Feb 24, 2021

alculquicondor left a comment

gavinfish commented Feb 24, 2021

alculquicondor commented Feb 24, 2021

alculquicondor Feb 25, 2021

tanjing2020 Feb 26, 2021

alculquicondor commented Feb 26, 2021

gavinfish Feb 27, 2021

tanjing2020 Mar 1, 2021

alculquicondor Mar 1, 2021

tanjing2020 commented Mar 5, 2021

tanjing2020 commented Mar 5, 2021

tanjing2020 commented Mar 5, 2021

alculquicondor Mar 5, 2021

alculquicondor left a comment

k8s-ci-robot commented Mar 8, 2021

alculquicondor commented Mar 8, 2021

Use %w instead of %v to format errors #99389

Use %w instead of %v to format errors #99389

Conversation

tanjing2020 commented Feb 24, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

tanjing2020 commented Feb 24, 2021

k8s-ci-robot commented Feb 24, 2021

lala123912 commented Feb 24, 2021

chendave left a comment

Choose a reason for hiding this comment

tanjing2020 commented Feb 24, 2021

gavinfish left a comment

Choose a reason for hiding this comment

alculquicondor commented Feb 24, 2021

alculquicondor left a comment

Choose a reason for hiding this comment

gavinfish commented Feb 24, 2021

alculquicondor commented Feb 24, 2021

alculquicondor Feb 25, 2021

Choose a reason for hiding this comment

tanjing2020 Feb 26, 2021

Choose a reason for hiding this comment

alculquicondor commented Feb 26, 2021

gavinfish Feb 27, 2021

Choose a reason for hiding this comment

tanjing2020 Mar 1, 2021

Choose a reason for hiding this comment

alculquicondor Mar 1, 2021

Choose a reason for hiding this comment

tanjing2020 commented Mar 5, 2021

tanjing2020 commented Mar 5, 2021

tanjing2020 commented Mar 5, 2021

alculquicondor Mar 5, 2021

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 8, 2021

alculquicondor commented Mar 8, 2021